Conversation
Entire-Checkpoint: 4157af44db5f
PR SummaryLow Risk Overview Extends Written by Cursor Bugbot for commit 8a0416e. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| } | ||
|
|
||
| b.ReportMetric(float64(time.Since(start).Milliseconds()), "ms/op") | ||
| } |
There was a problem hiding this comment.
ReportMetric in loop only keeps last iteration's value
Medium Severity
b.ReportMetric overwrites any previously reported value for the same unit, so calling it inside the for range b.N loop means only the last iteration's latency is reported as ms/op, discarding all earlier measurements. The total time needs to be accumulated across all iterations and divided by b.N after the loop ends, then reported once with a single b.ReportMetric call. With -benchtime=5x, 4 out of 5 samples are silently thrown away.
There was a problem hiding this comment.
Pull request overview
This PR adds initial benchmarking infrastructure to measure the performance of the entire hooks claude-code session-start command under various scaling conditions. The benchmarks help identify performance bottlenecks as repositories grow in size, number of sessions, refs, and commit history.
Changes:
- Added integration benchmark suite for hook session-start latency measurements
- Extended benchutil with helpers to seed branches and pack refs for realistic testing scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/entire/cli/integration_test/hook_bench_test.go | New benchmark suite testing session-start hook performance across four scaling dimensions: session count, ref count, repo files, and commit history |
| cmd/entire/cli/benchutil/benchutil.go | Added SeedBranches and PackRefs helper methods to simulate repos with many git refs for benchmark testing |
| claudeProjectDir := b.TempDir() | ||
|
|
||
| b.ResetTimer() | ||
| for range b.N { |
There was a problem hiding this comment.
This benchmark uses the older for range b.N pattern instead of the newer b.Loop() pattern that was introduced in Go 1.24. The codebase is using Go 1.25.6 and all other benchmarks in benchutil_test.go consistently use b.Loop().
While for range b.N still works, using b.Loop() is the modern approach and provides better ergonomics. Consider updating this to match the pattern used throughout the codebase for consistency.
| start := time.Now() | ||
|
|
||
| cmd := exec.Command(binary, "hooks", "claude-code", "session-start") | ||
| cmd.Dir = repo.Dir | ||
| cmd.Stdin = bytes.NewReader(stdinPayload) | ||
| cmd.Env = append(os.Environ(), | ||
| "ENTIRE_TEST_CLAUDE_PROJECT_DIR="+claudeProjectDir, | ||
| ) | ||
|
|
||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| b.Fatalf("session-start hook failed: %v\nOutput: %s", err, output) | ||
| } | ||
|
|
||
| b.ReportMetric(float64(time.Since(start).Milliseconds()), "ms/op") |
There was a problem hiding this comment.
Using manual timing with time.Now() and b.ReportMetric alongside b.ResetTimer() creates competing timing mechanisms. When using b.Loop(), the testing framework automatically handles timing, eliminating the need for manual time.Now()/time.Since() calls and b.ReportMetric().
If you switch to b.Loop() as suggested, you can remove the manual timing code (lines 142, 156) and let the framework handle it automatically. The framework's timing is more accurate as it accounts for framework overhead.
1 participant
Entire-Checkpoint: 4157af44db5f